Skip to content

Add socket timeout integration tests#648

Merged
rschmitt merged 1 commit into
apache:masterfrom
rschmitt:so-timeout
Jun 16, 2025
Merged

Add socket timeout integration tests#648
rschmitt merged 1 commit into
apache:masterfrom
rschmitt:so-timeout

Conversation

@rschmitt

@rschmitt rschmitt commented Jun 9, 2025

Copy link
Copy Markdown
Contributor

This change adds integration test coverage for various types of read timeouts. The test coverage includes HTTP, HTTPS, and HTTP over UDS if available. The /random request handlers have been augmented with delay support, so that the clients can read the response headers and then time out while reading the entity. The tests make use of ConnectionConfig, ResponseTimeout, and (on the sync client only) SocketConfig, and demonstrate the precedence among these config options when more than one is supplied.

One issue uncovered by these tests is that graceful closure of the async client does not work when UDS socket timeouts have fired. To work around this until the issue can be root caused, the async UDS tests use CloseMode.IMMEDIATE.

@rschmitt

rschmitt commented Jun 9, 2025

Copy link
Copy Markdown
Contributor Author

I was startled to learn that java.net.Socket::setSoTimeout only sets read timeouts:

https://github.com/openjdk/jdk/blob/d186dacdb7b91dc9a28b703ce3c8ea007fc450b6/src/java.base/share/classes/java/net/Socket.java#L1258-L1260

Given that this is the case, I'm not sure what prevents synchronous clients from going to sleep forever if a write blocks indefinitely because the send buffer filled up. Additionally, I haven't found a way to simulate this condition using localhost (at least not on macOS), so I can't even determine whether there's a sensible default value or something.

@ok2c

ok2c commented Jun 10, 2025

Copy link
Copy Markdown
Member

I was startled to learn that java.net.Socket::setSoTimeout only sets read timeouts:

https://github.com/openjdk/jdk/blob/d186dacdb7b91dc9a28b703ce3c8ea007fc450b6/src/java.base/share/classes/java/net/Socket.java#L1258-L1260

Given that this is the case, I'm not sure what prevents synchronous clients from going to sleep forever if a write blocks indefinitely because the send buffer filled up. Additionally, I haven't found a way to simulate this condition using localhost (at least not on macOS), so I can't even determine whether there's a sensible default value or something.

@rschmitt In the older version of HttpClient the socket timeout used to be called SO_READ_TIMEOUT. This is yet another limitation of the classic i/o one must tolerate

This change adds integration test coverage for various types of read
timeouts. The test coverage includes HTTP, HTTPS, and HTTP over UDS if
available. The `/random` request handlers have been augmented with delay
support, so that the clients can read the response headers and then time
out while reading the entity. The tests make use of `ConnectionConfig`,
`ResponseTimeout`, and (on the sync client only) `SocketConfig`, and
demonstrate the precedence among these config options when more than one
is supplied.

One issue uncovered by these tests is that graceful closure of the async
client does not work when UDS socket timeouts have fired. To work around
this until the issue can be root caused, the async UDS tests use
`CloseMode.IMMEDIATE`.
@rschmitt

Copy link
Copy Markdown
Contributor Author

This is yet another limitation of the classic i/o one must tolerate.

Yeah, this is probably one reason why virtually all server IO is non-blocking, even when the API is synchronous (see for example Tomcat).

Comment on lines +160 to +162
// TODO: Why does `CloseMode.GRACEFUL` (the test client default)
// block until the IOReactor shutdown timeout when using UDS?
client.close(CloseMode.IMMEDIATE);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still need to look into this some more. Based on the logspam I get during IOReactor shutdown, it's some sort of low-level selector issue. Could be a behavioral difference with UDS SocketChannels.

@rschmitt

Copy link
Copy Markdown
Contributor Author

@ok2c Anything else on this one?

@ok2c ok2c left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rschmitt Not really. Looks good to me

@rschmitt rschmitt merged commit 06481a0 into apache:master Jun 16, 2025
10 checks passed
@rschmitt rschmitt deleted the so-timeout branch June 16, 2025 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants